feat(cast): erc20 safe preflight#12499
Conversation
Syzygy106
requested review from
0xrusowsky,
DaniPopes,
grandizzy,
mattsse,
onbjerg and
zerosnacks
as code owners
|
Also, I would like to update Foundry book, according to a new flag |
grandizzy
left a comment
grandizzy
left a comment
There was a problem hiding this comment.
thank you, left a comment re new arg, don't think we need it?
grandizzy
changed the title
|
Hmmm, CI / deny fails. But looks like it isn't my fault |
grandizzy
left a comment
grandizzy
left a comment
There was a problem hiding this comment.
thank you, left comment, pls check
onbjerg
left a comment
onbjerg
left a comment
There was a problem hiding this comment.
i know you can pipe in yes, but let's also add a --yes/-y as this is pretty common for clis
Hi there, @onbjerg , I actually agree with the However, other maintainers had different perspectives:
Their reasoning was avoiding adding new arguments to keep the CLI simpler and easier to maintain and following Unix philosophy with I'm happy to implement either approach, but before making changes again, I think it would be helpful if the maintainers could align on the preferred solution: Option 1: @grandizzy @0xrusowsky @onbjerg - could you please discuss and let me know which direction you'd like me to take? I want to make sure we're all aligned before implementing the final version. Other maintainers are also invited to the conversation @mattsse , @zerosnacks , @DaniPopes |
|
@Syzygy106 sorry for back and forth, we'll discuss and make a final call re how to add this. thank you! |
|
@Syzygy106 sorry, my bad, should have been get full consensus on it. Let's stick with your initial impl, that is adding the --yes/-y arg. Thank you |
Yep, no problem! |
|
@onbjerg , @grandizzy , done |
@grandizzy, could you please review the PR when you have a moment? |
Merged confirmation prompt feature (--yes flag) with master's updates: - Combined --yes flag for transfer/approve with curl support and tx_opts - Added --yes flag to all test cases that use transfer/approve - Kept both PR's confirmation tests and master's gas/curl tests Amp-Thread-ID: https://ampcode.com/threads/T-019bdbc8-2e58-7396-9320-bbd9c1933a27 Co-authored-by: Amp <amp@ampcode.com>
The tests were checking that output starts with 0x (tx hash), but cast_send outputs the full receipt in sync mode. Updated tests to just verify the transaction succeeded via balance/allowance checks.
gakonst
force-pushed
the
feat/cast-erc20-safe_preflight
branch
from
4cdf238 to
65ea3f2
Compare
7 participants
Motivation
Closes #12402
Enhance UX and prevent accidental transfers with incorrect amounts. Users may misunderstand the token's decimal places and accidentally send far more (or less) tokens than intended.
Also, approve() requires identical feature, so I decided to add the same.
At the other hand, transferFrom() - doesn't need that, because this method is rarely called by users implicitly, more often him calls other smartcontracts (like swapRouter).
Solution
Added interactive confirmation prompts to cast erc20 transfer and cast erc20 approve commands that:
Fetch token metadata (symbol and decimals) from the ERC20 contract
Display human-readable amounts in the confirmation prompt:
Example: Confirm transfer of 100 USDC to address 0x666...666 instead of raw 100000000
Provide --yes flag to skip confirmation for non-interactive usage (scripts, CI/CD)
Handle edge cases gracefully:
-Falls back to raw amount if decimals/symbol can't be fetched
-Shows warning if token metadata is unavailable
Demonstration
Let's create a test token first:
Then we can test behavior:
In case y (yes):
In case n (no):
With skip promt that would be:
In case where contract address aren't correct/contract doesn't support ERC20 interface for decimals/symbol, we'll get a warning:
The same behavior has been added to approve():
PR Checklist